Skip to content

fix(middleware): align matcher validation#2599

Open
james-elicx wants to merge 9 commits into
mainfrom
codex/security-app-route-cookie-cache-v2
Open

fix(middleware): align matcher validation#2599
james-elicx wants to merge 9 commits into
mainfrom
codex/security-app-route-cookie-cache-v2

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • align matcher parsing and request-condition semantics with Next.js
  • reject unsafe matcher expressions without blocking common bounded patterns
  • preserve matcher behavior across development, Node, and Workers

Testing

  • matcher and request-condition unit coverage
  • development, Node production, and Worker auth fixtures
  • timed regex-safety subprocess coverage
  • vp check
  • vp run vinext#build

@pkg-pr-new

pkg-pr-new Bot commented Jul 12, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2599
npm i https://pkg.pr.new/create-vinext-app@2599
npm i https://pkg.pr.new/vinext@2599

commit: 95d62e5

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 95d62e5 against base 8d0a18d using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 4 regressed · 2 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 126.6 KB 129.6 KB 🔴 +2.4%
Client entry size (gzip) vinext 120.6 KB 123.6 KB 🔴 +2.5%
Dev server cold start vinext 2.59 s 2.57 s ⚫ -0.8%
Production build time vinext 2.65 s 2.65 s ⚫ +0.2%
RSC entry closure size (gzip) vinext 98.6 KB 101.6 KB 🔴 +3.0%
Server bundle size (gzip) vinext 165.3 KB 168.3 KB 🔴 +1.8%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(middleware): align matcher validation

Reviewed all 30 files. This is a solid, security-focused refactor that replaces two hand-rolled ReDoS heuristics with a deterministic structural analyzer plus a faithful port of path-to-regexp 6.3, and validates matchers at config time. Test coverage is excellent — the new shims.test.ts case compiles every fixture pattern with next/dist/compiled/path-to-regexp and asserts vinext produces the identical RegExp.source/flags, and the auth fixture exercises dev, Node prod, and Workers. I ran tests/shims.test.ts (1214 pass) and tests/middleware-matcher-auth.test.ts (27 pass), plus vp check on the changed source files (clean).

Verified the key parity claims against Next.js canary (.nextjs-ref):

  • _matchConditionValue / query has semantics match matchHas in prepare-destination.ts: empty/omitted value is a presence check requiring a non-empty actual value, duplicate query keys select the last element (value.slice(-1)[0]), and a duplicate key is truthy even when its final value is empty.
  • middleware-path-to-regexp.ts token output and compiled regex match the vendored path-to-regexp, including the normalizeMiddlewarePathTokens slash-prefix retry.

No blocking issues found. A few observations:

Observations (non-blocking)

  1. Two ReDoS analyzers now coexist. unsafeTokenReason runs both the AST-based analyzeRegexSafety (which already reports "overlapping sequential repetition") and the separate hand-rolled string scanner hasOverlappingSequentialRepetition in middleware-matcher-pattern.ts:117. Both leaning toward false-positives (fail-closed) is fine as belt-and-suspenders, but it's a maintenance burden — two code paths that must stay in agreement. Worth a follow-up to consolidate on the AST analyzer if it can cover the string-scanner cases, or a comment explaining why both are retained.

  2. Config headers/redirects/rewrites still use the old tokenizer. matchConfigPattern in config-matchers.ts:724+ retains the legacy hand-rolled tokenizer + extractConstraint, so only middleware matchers gained the path-to-regexp-parity implementation. This is consistent with the PR's scope ("middleware matcher validation"), and those config paths do benefit from the stricter shared analyzeRegexSafety via safeRegExp. Flagging only so it's a conscious decision — config-route matching still diverges from Next.js's path-to-regexp semantics.

  3. isSafeRegex is now strictly stricter for all callers. Since safeRegExp powers config header/redirect/rewrite compilation too, patterns that previously passed the naive nested-quantifier heuristic may now be rejected (returning null). All call sites I checked handle null gracefully (fall back to linear scan / non-match / warn), and the rejected patterns are genuinely backtracking-prone, so the direction is safe. Callers relying on previously-accepted pathological patterns would see a behavior change, but that's the intended hardening.

  4. matchesMiddleware fail-open shift is correct. Non-array matcher configs and invalid matcher objects now return true (middleware runs for all paths) instead of false. This is the security-correct direction (don't silently bypass an auth guard) and Next.js validates these at build time anyway, so runtime only sees them as defense-in-depth.

Performance note

The benchmark comment reports ~+1.8–3.0% growth across server/client/RSC bundle sizes, attributable to the new 899-line regex-safety.ts + 312-line pattern module + 281-line path-to-regexp. Given AGENTS.md's guidance to keep the common request path lightweight, consider whether regex-safety.ts (only needed at matcher-compile time, which is cached per pattern) can be lazily imported off the hot request path rather than statically pulled into the shared RSC/SSR/worker closures. Not blocking, but the size regression is measurable.

Overall: correct, well-tested, and faithful to Next.js. Approving direction; the items above are follow-ups rather than fixes required for merge.

@ask-bonk

ask-bonk Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2599.

Summary

I reviewed all 30 files in PR #2599 (fix(middleware): align matcher validation), verified parity claims against Next.js canary source, and ran the relevant test suites.

Verdict: No blocking issues. The PR is correct, well-tested, and faithful to Next.js.

What I verified:

  • Ran tests/shims.test.ts (1214 pass), tests/middleware-matcher-auth.test.ts (27 pass, covering dev/Node/Workers), and vp check on all changed source files (clean).
  • Confirmed the matchHas query/header/cookie semantics against Next.js prepare-destination.ts (presence checks, duplicate-key slice(-1)[0] handling).
  • Confirmed middleware-path-to-regexp.ts produces byte-identical regexes to the vendored path-to-regexp (the new shims test asserts this directly).
  • Confirmed the matchesMiddleware fail-open shift (invalid/non-array matchers now return true) is the security-correct direction.

Non-blocking observations I raised:

  1. Two ReDoS analyzers now coexist (AST-based + hand-rolled string scanner) — maintenance burden worth consolidating.
  2. Config headers/redirects/rewrites still use the old tokenizer — only middleware got path-to-regexp parity (consistent with PR scope).
  3. isSafeRegex is now strictly stricter for all callers, but every call site handles rejection gracefully.
  4. The ~+1.8–3.0% bundle-size regression from the new modules could be mitigated by lazy-importing regex-safety.ts off the hot request path, per AGENTS.md guidance.

github run

@james-elicx james-elicx marked this pull request as ready for review July 13, 2026 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant